Skip to content
This repository has been archived by the owner on Jun 23, 2023. It is now read-only.

Add proper SysEx support to PortMidi #526

Merged
merged 4 commits into from
Sep 2, 2022
Merged

Add proper SysEx support to PortMidi #526

merged 4 commits into from
Sep 2, 2022

Conversation

ceski-1
Copy link
Contributor

@ceski-1 ceski-1 commented Sep 2, 2022 •
edited
Loading

Implementing this also has the benefit of fixing a number of related issues.

Changes:

  • Process SysEx events correctly from midi files so music sounds as intended
  • Send two SysEx messages, one reset and one master volume, and a smaller set of control changes for compatibility with MS GS Wavetable Synth
  • Stop function no longer called twice on shutdown
  • Volume no longer initializes twice and incorrectly the first time
  • Reset midi device on launch, shutdown, and whenever a song changes
  • Configure midi device to GS, GM, GM2, or XG modes
  • Configure delay after reset for hardware midi devices (e.g. Roland SC-55)
  • Fix regression: wads with mid-level music changes no longer cause the game to pause briefly (test wad)
  • Music changes no longer result in occasional "chirp" sound
  • Volume levels no longer set two different ways and too low when using the Windows setting "mus_extend_volume"

New settings in prboom-plus.cfg:

mus_portmidi_reset_type "gs" (gs, gm, gm2, xg)
Reset type. Default is fine for most users.

mus_portmidi_reset_delay 0 (0 to 2000 ms)
Delay after reset. Default is fine for software devices like MS GS Wavetable Synth, VirtualMIDISynth, or Roland Sound Canvas VA. For hardware devices like the Roland SC-55, try around 100 to 300 ms.

mus_portmidi_filter_sysex 0 (0 or 1)
Filter (block) SysEx messages from midi files. Default (0 = don't filter) allows midi files to sound as intended. Pr+ couldn't process SysEx messages before and enabling this setting replicates that behavior at the expense of music accuracy.

@ceski-1
Copy link
Contributor Author

ceski-1 commented Sep 2, 2022

Here's a capture from Alien Vendetta map04 to help visualize things:
av_map04

From top to bottom:

  1. GS Reset from Pr+ PortMidi.
  2. Master Volume from Pr+ PortMidi, scaled to music slider.
  3. GS Reset from midi file. Redundant but many midi files don't include a reset.
  4. Pr+ PortMidi detects GS Reset and reapplies Master Volume.
  5. Reverb Level set to 80 from midi file.
  6. Reverb Time set to 80 from midi file.
  7. Ch. 12 Pan set to 0 from midi file.
  8. Voice Reserve tweaks from midi file to avoid notes cutting off.
  9. Various channels configured just before notes begin to play.

@rfomin
Copy link
Contributor

rfomin commented Sep 2, 2022

The problem with this PR is that MS GS Wavetable Synth does not support SysEx messages if I remember correctly. Try to test the examples from here: #101 (comment)

@ceski-1
Copy link
Contributor Author

ceski-1 commented Sep 2, 2022

MS GS Wavetable Synth supports SysEx but misses some settings when executing a reset. I made some changes to address this and other odd cases that may occur. It passes the pitch bend range tests that you linked and sounds good now. Thank you.

@rfomin
Copy link
Contributor

rfomin commented Sep 2, 2022

Thank you! This PR is very interesting, I guess I did something wrong when testing SysEx messages when writing i_winmusic.c for Woof/Choco/Crispy.

@ceski-1
Copy link
Contributor Author

ceski-1 commented Sep 2, 2022

No problem! I was going to look at those projects next. It's a little trickier without PortMidi though.

@rfomin
Copy link
Contributor

rfomin commented Sep 2, 2022

It's a little trickier without PortMidi though.

You need to use midiOutLongMsg() and prepare header for it first. It's a bit inconvenient, but shouldn't be difficult. I'll try this weekend too.

@kraflab
Copy link
Collaborator

kraflab commented Sep 2, 2022

It looks good to me. Any further concerns @rfomin @fabiangreffrath ?

fabiangreffrath
fabiangreffrath approved these changes Sep 2, 2022
View reviewed changes
Copy link
Collaborator

@fabiangreffrath fabiangreffrath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@fabiangreffrath fabiangreffrath merged commit af82970 into coelckers:master Sep 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kraflab kraflab kraflab left review comments

@fabiangreffrath fabiangreffrath fabiangreffrath approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ceski-1 @rfomin @kraflab @fabiangreffrath